-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change Cancel() to async in InlineRenameSession. #74946
Conversation
|
||
// This wait is safe. We are not passing the async callback to DismissUIAndRollbackEditsAndEndRenameSessionAsync. | ||
// So everything in that method will happen synchronously. | ||
DismissUIAndRollbackEditsAndEndRenameSessionAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key change in the PR.
I feel we don't need _threadingContext.ThrowIfNotOnUIThread();
because in DismissUIAndRollbackEditsAndEndRenameSessionAsync
it immediately switches to main thread.
@@ -223,7 +224,7 @@ public bool Submit() | |||
public void Cancel() | |||
{ | |||
SmartRenameViewModel?.Cancel(); | |||
Session.Cancel(); | |||
_ = Session.CancelAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not a fan of this being fire-forget here. i think we shoudl make the containing call async, and decide waht to do (on a case by case basis) for every caller. Some will be able to await, some can be explicit fire-and-forget, and some can block.
Note: Fire-and-forget here is not good for rename tracking as we have untracked async work being kicked off. anywhere we do this, we need IAsyncOpListener hooked up to keep track of things.
private async Task CancelAsync() | ||
{ | ||
//.ConfigureAwait(true) because we need to set focus later | ||
await _model.Session.CancelAsync().ConfigureAwait(true); | ||
_textView.VisualElement.Focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not set focus on the _textView prior to cancelling the session (which seems like it could take unbounded time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stepping back (and maybe this pr will calrify it), it's unclear to me why cancelling (which just should cancel some CT tokens, and return the buffers to the initial state) would be async. IT seems like it should be fast and UI oriented. not sure why cancelling itself is async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I've made a different change here: #74948
Cancellation here should be synchronous here.
There is a Task.Wait() in Cancel() method of inline rename session.
This PR removes it and make all other calls to be async